Agent policy legibility, plus the Windows stall bisect - #120
Merged
b-macker merged 4 commits intoAug 4, 2026
Conversation
Three usability defects found while tracing the agent policy surface. All three were reported from real application use; each is traced to a line and each fix was verified against a stub, not just built. 1. The shell response scan claimed execution that never happened. agent_impl.cpp gates a RESPONSE CONTENT scan on shell_allowed, the same flag checkShellAllowed() uses for real execution, then reports "Shell execution is blocked for this agent role". Nothing was executed. The scan matches '$ <command>' lines, so an agent restricted from shell cannot write install instructions for ANY language — verified: a response containing "$ npm install express" is HARD-blocked. The message now says what was actually matched and states that nothing ran. Splitting the flag into execution and content halves is a schema change and is NOT done here. 2. Per-agent policy refusals did not name the agent. All seven omitted it while 30 of 111 "Agent error:" throws in the same file DO format in config_name — including the response-scan errors three functions away. Two conventions, and the refusals picked the wrong one. With 13 agents configured, "Action matrix does not include AGENT_SEND" is unattributable. Now names the agent and the exact govern.json path. The agent name is already in the config the operator wrote, so this leaks nothing. The agent.create refusal also now says WHICH sense of AGENT_SEND it means: the permission is overloaded, granting "may be sent to" at agentSend and "may spawn children" at agentCreate, and the old text did not distinguish. 3. Nine environment state keys vanished instead of defaulting. dict.get() returns null for both "absent" and "zero", so no NAAb script can tell them apart. Adjacent keys disagreed: tool_integrity_count has an explicit zero on its else path, tool_calls_total simply disappeared when tools were off. The defaults are NOT uniform, deliberately. 0 is honest for the tool counters — tools disabled means no calls were made. For the lease and budget keys 0 is a lie: it is exactly what an EXPIRED lease and an EXHAUSTED budget report, so those default to -1, the sentinel already used by escalation_turn and by tokens_remaining's own unlimited case. Verified per fix against the agent stub: the shell error prints the new wording, the refusal prints "Agent 'deploy_lead' — action matrix ...", and a no-tool no-lease agent now reports tool_calls_total=0, lease_remaining=-1, risk_budget_remaining=-1 instead of three missing keys. 441 tests / 0 unexpected, 874 leak checks / 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ELUfjXZvx8kzXo1UJjrAhC
NAAb Governance Report
All governance checks passed! Generated by NAAb Governance Engine v4.0 |
Three consecutive CI runs failed in a stub-backed suite, each time a DIFFERENT one, none reproducible locally: #119 Build & Test test_quarantine_corroboration.sh #120 build-linux test_failure_mode_coverage.sh #120 Build & Test test_absorption_degenerate.sh A code regression fails the same test every run. A different test each time, all sharing one start_stub idiom copied across 9 files, is the launcher. Two defects in it, both of which bite only under load: - the port is picked at random with no bind check and no retry, so a collision or a lingering TIME_WAIT socket leaves the stub dead - the readiness wait was a flat 5s, and python3 startup + bind exceeds that often enough to matter on a busy runner Either way start_stub returned 1, the caller printed STUB_FAIL, and every assertion in the suite then failed for a reason unrelated to what the test measures — with nothing in the output saying so. Now retries across 3 ports, waits 30s per attempt, and prints the stub log tail naming the real cause. The kill -0 check is what keeps the longer bound cheap: a stub that died on bind is detected immediately and retried on a fresh port instead of waiting out the ceiling. Verified the retry path by making every bind fail: 3 attempts, the actual error surfaced ("Address already in use"), rc=1 — in 0 seconds, not 90. The old code burned 5s silently and said nothing. All 9 patched suites pass individually (93 assertions), 441 tests / 0 unexpected, 874 leak checks / 0 failures. Not claimed: that this is the whole cause. It is the mechanism that fits every observation, and the next failure now names itself either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ELUfjXZvx8kzXo1UJjrAhC
d05a65d caused the Windows stall it was meant to help diagnose. The bisect is one commit wide and unambiguous: 056e659 policy refusals + env keys, no stub change build-windows PASSED d05a65d stub launcher hardening, 9 test files only build-windows STALLED The entire delta between them is the launcher. Two defects, both confined to the FAILURE path — a stub that comes up promptly never enters it, which is why Linux stayed green and why Windows was fine until the retry existed: - `kill` (TERM) followed by an unbounded `wait`. run-all-tests.sh already warns in its own comments that native Windows binaries under MSYS2 ignore TERM and that plain `timeout` "can wait forever". A `wait` on a child that never dies does not return, and a process tree holding an unkillable child is also why the runner could neither enforce `timeout-minutes: 25` (it ran 21m past the bound) nor finalize the step nor upload logs. - Spawn amplification. 50 iterations of grep+sleep became 3x300 iterations of grep+kill+sleep — roughly 100 process spawns to ~2700. At the ~50-100ms fork/exec cost of MSYS2 the loop is dominated by spawning, not by the 30s of intended waiting. Windows now takes the single 5s attempt that ran green for many jobs: no retry, no kill, no wait. POSIX keeps the retry, with sleep 0.5 over 60 iterations so the longer ceiling costs no more spawns than the original, and SIGKILL with no wait at all — reaping is not worth a hang. Verified both paths against a stub rigged to fail every bind: POSIX retries 3 ports, surfaces the real error and returns in 2s; Windows takes one 5s attempt and returns, byte-for-byte the pre-d05a65d behaviour. All 9 suites pass (93 assertions), 441 tests / 0 unexpected, 874 leak checks / 0 failures. The three stalls that predate d05a65d are NOT explained by this and remain open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ELUfjXZvx8kzXo1UJjrAhC
…the tree test_absorption_degenerate.sh has carried this comment since before any of today's work: "Stub-backed HTTP tests hang on Windows/MSYS2 due to signal propagation and process cleanup issues. Skip entirely — Linux CI validates the behavior." The diagnosis was made once and the guard applied to ONE file. 29 suites launch the stub; 28 of them still ran on Windows. build-windows has stalled four times since, always inside "CLI tests — shell suites", each time sitting in_progress ~47 minutes before the runner is killed service-side with the log archive 404ing. timeout-minutes failed to fire on two of those, so the runner cannot enforce its own step timeout either: the cause cannot be read out, only excluded. All 29 are now excluded on Windows, on both `uname -s` and $WINDIR. Read the next Windows run as the experiment's result: green -> stub-backed suites are the cause; narrow from 29 stalls -> they are exonerated and the cause is elsewhere in the phase Guarding only the 9 I first found would have made a "stalls" result uninterpretable, since 20 unguarded launchers would still have been running. That set of 9 was an artifact of grepping for ONE launcher idiom rather than for agent_stub.py — the same mistake that hid the reload_count guard earlier today. Coverage is not lost: build-linux and Build & Test run all 29 in full, and agent-governance semantics are platform-neutral. Reversible in one commit. Verified: all 29 observed taking the skip under a simulated MINGW uname (not merely assumed to), 441 tests / 0 unexpected and 874 leak checks / 0 failures unchanged on Linux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ELUfjXZvx8kzXo1UJjrAhC
b-macker
marked this pull request as ready for review
August 4, 2026 01:31
4 tasks
b-macker
added a commit
that referenced
this pull request
Aug 4, 2026
The 29 guards added in #120 carried "pending stall bisect" in their comments. The bisect is finished: excluding all 29 took "CLI tests — shell suites" from a 47-minute hang to 2m04s. Replaces the near-copies with tests/helpers/stub_platform.sh — 762 lines removed, 58 added. The line count is not the point. The reason the stall took four occurrences to diagnose is that the finding was recorded in ONE file's comment, where the 28 other callers of the same launcher could not see it. A single definition is what makes the next such finding reach all of them. The helper states what is known rather than what was suspected, and says plainly that it is a workaround: the launcher's signal and cleanup behaviour under MSYS2 is the actual defect and remains open. Also records the episode in docs/governance-campaign-findings.md, which exists so the next person does not rediscover what was already settled — precisely the failure this entry is about. New section "A fix that reached one caller", with measured figures: the Windows guard reached 1 of 29 suites, and 081f460's stub port retry reached 2 of 29. Two withdrawn proposals (the agent.commit() gap that does not exist; the launcher hardening whose one-commit bisect was coincidence), and a fifth method note on how a grep defines what you are able to see. Verified in both directions: all 29 observed taking the skip under a simulated MINGW uname and under $WINDIR, and confirmed NOT firing on Linux. 441 tests / 0 unexpected, 874 leak checks / 0 failures, suite total unchanged.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three usability defects on the agent policy surface, all reported from real application use and each traced to a line and verified against the agent stub before being changed. Three follow-up commits then chased a CI stall that blocked the PR and ended up locating it.
1. The shell response scan claimed an execution that never happened
agent_impl.cppgates a response content scan onshell_allowed— the same flagcheckShellAllowed()(governance_engine.cpp:1870) uses for real execution — then reportedShell execution is blocked for this agent role. Nothing was executed. Reproduced against the stub withcapabilities.shell.enabled: trueglobally and only the agent'sshell_allowed: false.The scan matches
$ <command>lines, so the blast radius is wider than "shell". Measured:$ npm install express# sudo apt installAn agent restricted from shell cannot write install instructions for any language —
npm,pip,apt,curl,wgetall sit in that pattern. The message now names what was matched and says plainly that nothing ran. Splitting the flag is a schema change and is not done here.2. Per-agent policy refusals did not name the agent
All seven omitted it, while 30 of 111
Agent error:throws in the same file do format inconfig_name— including the response-scan errors three functions away. Two conventions; the refusals picked the wrong one. With 13 agents configured,Action matrix does not include AGENT_SENDis unattributable.The
agent.createrefusal additionally says which sense ofAGENT_SENDit means, because the permission is overloaded: atagentSend:1789it grants "may be sent to", atagentCreate:1072"may spawn children".3. Nine environment
statekeys vanished instead of defaultingdict.get()returns null for both "absent" and "zero". Adjacent keys disagreed:tool_integrity_counthas an explicit zero on its else path,tool_calls_totalsimply disappeared when tools were off.The defaults are deliberately not uniform:
tool_calls_total,_blocked,_latency_mslease_remaining,_secondsturns_remaining,tokens_remainingrisk_budget_remainingDefaulting the lease and budget keys to 0 replaces a silent absence with a confident lie.
-1is the sentinel already used byescalation_turn(837) andtokens_remaining's own unlimited case (697).4–6. The Windows stall, and what actually found it
build-windowshad stalled four times insideCLI tests — shell suites: stepin_progress~47 minutes, runner killed service-side, log archive 404.timeout-minutesfailed to fire on two of those, so the runner could not enforce its own step timeout — the cause could not be read out, only excluded.Two wrong turns, recorded because they cost the most time:
d05a65dhardened the stub launcher (port retry, longer readiness ceiling) after three Linux suites failed across two runs. It then stalled Windows, and a one-commit bisect (056e659passed →d05a65dstalled, delta = launcher only) looked conclusive. It wasn't.79a7f20restored the Windows path to byte-equivalent prior behaviour and stalled anyway. The bisect was coincidence.agent_stub.py. The real population is 29. Excluding 9 would have left 20 running and made a negative result uninterpretable.The answer was already in the tree.
test_absorption_degenerate.shhas carried this since before any of this work:The diagnosis was made once and applied to one file out of 29.
e5c6f00applies it to all of them, and the result is unambiguous:CLI tests — shell suitesin_progress47 min → runner killedCoverage is not lost:
build-linuxandBuild & Testrun all 29 in full, and agent-governance semantics are platform-neutral. Reversible in one commit.Retraction
An earlier revision of this description claimed a security gap: that
agent.commit()re-checks lease and CRITICAL but notallowed_actions, so a proposal could be committed afterAGENT_SENDwas ratcheted away. That is wrong and the gap does not exist.agentCommitalready carries a reload-generation check (getReloadCount() != selected.reload_count) stamped at propose time.reload_count_++fires only on an accepted reload (governance_config.cpp:3873), andallowed_actionsis config-derived, so it cannot change without invalidating the proposal first.s_pending_proposalshas exactly one insert — insideagentPropose, behind its ownAGENT_SENDcheck — so a caller cannot supply a proposal either. Covered bytest_propose_commit.shH-01/H-02, which pass 25/25.I missed it by grepping
allowed_actions|AGENT_SEND|checkCriticalSuspension|leaseExpiredLocked|checkAdmission—reload_countwas not in the pattern, so the guard was invisible. Same grep-shaped-model error as the 9-vs-29 count above.Test Plan
bash run-all-tests.shwith no new failures — 441 tests, 0 unexpected failuresbash tests/security/test_error_msg_leaks.sh— 874 checks, 0 failures (error text changed, so this one matters)Verified per fix against the agent stub, before and after:
$ <cmd>patternAction matrix does not include AGENT_SENDAgent 'deploy_lead' — action matrix does not include AGENT_SENDtool_calls_total=0,lease_remaining=-1,risk_budget_remaining=-1All 29 guards were observed taking the skip under a simulated MINGW
uname, not assumed to fire. A static coverage check flagged 4 as unguarded; that was a false positive matching a header comment above the guard, and the dynamic check settled it.Follow-ups not in this PR
pending stall bisectcomments in the 29 guards describe a hypothesis that is now resolved; they should be rewritten to state the finding and consolidated into a shared guard.AGENT_SENDandshell_allowedare both overloaded; splitting either is a govern.json schema break.$ <cmd>pattern is a behaviour change to a security check and needs a control proving real shell content still blocks.Generated by Claude Code